feat(cpflow): add ordered renderer workload#777
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
WalkthroughThe deployment configuration now separates the React on Rails Pro renderer into its own workload, adds rolling-deploy environment and secret handling, updates Rails startup configuration, pins CPFlow integrations to v5.2.0, and revises related deployment documentation. ChangesControl Plane renderer rollout
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ControlPlane
participant NodeRenderer
participant Rails
ControlPlane->>NodeRenderer: Deploy and warm renderer cache
NodeRenderer->>NodeRenderer: Start renderer on port 3800
ControlPlane->>Rails: Deploy after node-renderer
Rails->>NodeRenderer: Connect through same-GVC DNS
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-07-13T09:40:34Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: terraformplan-json scan error: fs filter error: fs filter error: walk error range error: stat doctor.config.json: no such file or directory: range error: stat doctor.config.json: no such file or directory Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: feat(cpflow): add ordered renderer workloadOverview: this PR splits the React on Rails Pro Node Renderer out into its own Overall this is a well-scoped, well-documented infra change (nice touch keeping the Dockerfile's combined-process Findings
Other things I checked, no issues found
|
|
How do we coordinate the staging and production deploys to the new setup? |
Review SummaryWhat this PR does: Splits the React on Rails Pro Node Renderer out of the combined Rails+renderer container into its own Overall: This is a well-scoped, carefully documented infra change with a thoughtful rollback/monitoring plan in the PR description. The
Security: The new Tests: No automated test changes — this is Control Plane/infra config, validated manually per the PR description ( |
Review SummaryThis PR splits the React on Rails Pro Node Renderer out of the combined Rails container into its own Control Plane What looks solid
Issues flagged inline
Other observations (not flagged inline)
Overall this is a well-documented, carefully-scoped infra change with good rollback/monitoring notes already in the PR description. The main open question is functional verification that |
Review SummaryWhat this PR does: Splits the React on Rails Pro Node Renderer out of the shared Rails container into its own Control Plane workload ( Strengths
Issues raised inline
Minor / non-blocking
SecurityNo concerns — secrets ( |
6b124b7 to
ea63606
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const config = { | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | ||
| serverBundleCachePath: process.env.RENDERER_SERVER_BUNDLE_CACHE_PATH || path.resolve(__dirname, '.node-renderer-bundles'), | ||
| host: process.env.RENDERER_HOST || 'localhost', |
There was a problem hiding this comment.
This host field is the linchpin of the whole separate-workload design — without it binding to 0.0.0.0, Rails could never reach the renderer over the same-GVC DNS name (node-renderer.<app>.cpln.local:3800) and SSR would fail entirely post-deploy. The PR's validation list only shows node -c renderer/node-renderer.js (a syntax check), which wouldn't catch an unsupported/misspelled config key being silently ignored by reactOnRailsProNodeRenderer(config).
Worth confirming host is a real, honored option in react-on-rails-pro-node-renderer@17.0.0-rc.9 (e.g. via the gem's own docs/changelog for the linked upstream PR, or a runtime smoke test hitting the renderer from another container) before this ships to review/staging, since a silent no-op here would break SSR without any obvious error at deploy time.
| if ENV["ROLLING_DEPLOY_TOKEN"].present? | ||
| config.rolling_deploy_adapter = ReactOnRailsPro::RollingDeployAdapters::Http | ||
| config.rolling_deploy_token = ENV.fetch("ROLLING_DEPLOY_TOKEN") | ||
| config.rolling_deploy_previous_urls = ENV["ROLLING_DEPLOY_PREVIOUS_URLS"].presence |
There was a problem hiding this comment.
Minor: config.rolling_deploy_previous_urls is being fed a single URL string built from {{APP_NAME}} (see .controlplane/templates/app.yml's ROLLING_DEPLOY_PREVIOUS_URLS), but the plural env var/config name suggests the upstream adapter may expect a comma-separated list or an array. Worth double-checking the ReactOnRailsPro::RollingDeployAdapters::Http adapter's expected format for this value so a single-URL string isn't silently mis-parsed (e.g., split on , and treated as one giant malformed URL, or ignored).
ReviewOverviewThis PR splits the React on Rails Pro Node Renderer out of the single-container Rails workload into its own Control Plane Strengths
Points worth verifying before rollout
SecurityNo secrets are hardcoded; Overall this is a thorough, well-documented change with sensible defaults; the main follow-up is confirming the two renderer-library config assumptions actually behave as expected at runtime. |
Greptile SummaryThis PR moves the React on Rails Pro renderer into an ordered Control Plane workload. The main changes are:
Confidence Score: 5/5This looks safe to merge after checking the secret rollout step for existing apps.
.controlplane/templates/app.yml Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CP as cpflow deploy-image
participant NR as node-renderer workload
participant Old as previous app endpoint
participant Rails as rails workload
CP->>NR: deploy renderer image
NR->>Old: pre-seed previous bundles
NR->>NR: start yarn node-renderer
CP->>NR: wait for readiness
CP->>Rails: deploy Rails and daily-task
Rails->>NR: SSR over same-GVC DNS
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CP as cpflow deploy-image
participant NR as node-renderer workload
participant Old as previous app endpoint
participant Rails as rails workload
CP->>NR: deploy renderer image
NR->>Old: pre-seed previous bundles
NR->>NR: start yarn node-renderer
CP->>NR: wait for readiness
CP->>Rails: deploy Rails and daily-task
Rails->>NR: SSR over same-GVC DNS
Reviews (1): Last reviewed commit: "fix(cpflow): align renderer cache and cp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - name: RENDERER_PASSWORD | ||
| value: cpln://secret/{{APP_SECRETS}}.RENDERER_PASSWORD |
There was a problem hiding this comment.
When this template is applied to an existing app whose secret dictionary does not yet contain ROLLING_DEPLOY_TOKEN, the new renderer workload inherits an unresolved secret and runs pre_seed_renderer_cache under set -e before opening port 3800. That can keep node-renderer from becoming ready, so deploy_order blocks the Rails rollout until the secret is added.
| require "rails_helper" | ||
|
|
||
| RSpec.describe "ReactOnRailsPro initializer" do | ||
| around do |example| | ||
| original_env = { | ||
| "ROLLING_DEPLOY_TOKEN" => ENV.fetch("ROLLING_DEPLOY_TOKEN", nil), | ||
| "ROLLING_DEPLOY_PREVIOUS_URLS" => ENV.fetch("ROLLING_DEPLOY_PREVIOUS_URLS", nil) | ||
| } | ||
| config = ReactOnRailsPro.configuration | ||
| original_config = { | ||
| rolling_deploy_adapter: config.rolling_deploy_adapter, | ||
| rolling_deploy_token: config.rolling_deploy_token, | ||
| rolling_deploy_previous_urls: config.rolling_deploy_previous_urls | ||
| } | ||
|
|
There was a problem hiding this comment.
original_config only snapshots/restores rolling_deploy_adapter, rolling_deploy_token, and rolling_deploy_previous_urls. load-ing the initializer inside each example also re-runs every other config.*= assignment in config/initializers/react_on_rails_pro.rb (server_renderer, renderer_use_fallback_exec_js, renderer_url, renderer_password, enable_rsc_support, rsc_bundle_js_file, rsc_payload_generation_url_path). Today that's harmless because none of those depend on env vars this spec mutates, so reloading reassigns the same values — but it's a latent test-isolation trap: if a future edit to the initializer makes any of those depend on mutable env/state, this spec will leak it into global ReactOnRailsPro.configuration for the rest of the suite. Consider snapshotting/restoring the whole config object (e.g. config.dup/clone, or iterating all public config accessors) instead of an explicit allowlist of three keys.
| args: | ||
| - bash | ||
| - -lc | ||
| - | | ||
| # The rake task warns and continues on bundle fetch misses so rollout | ||
| # does not wedge; unexpected task failures still stop the shell. | ||
| set -e | ||
| bundle exec rake react_on_rails_pro:pre_seed_renderer_cache | ||
| exec yarn node-renderer |
There was a problem hiding this comment.
Since this workload keeps the Dockerfile's ENTRYPOINT ["./.controlplane/entrypoint.sh"] (only args/CMD is overridden), the renderer container will run entrypoint.sh's wait_for_service loop against DATABASE_URL and REDIS_URL before this script ever runs — even though the renderer itself doesn't talk to Postgres/Redis. That's extra startup latency stacked on top of the 120s startupProbe window, and if Postgres/Redis are ever slow to come up during first-time setup-app bootstrap, the renderer (and therefore the rails deploy behind it in deploy_order) stalls waiting on dependencies it doesn't need. Worth confirming this coupling is intentional, or giving node-renderer its own lightweight entrypoint/CMD that skips the DB/Redis wait.
|
|
||
| const config = { | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | ||
| serverBundleCachePath: process.env.RENDERER_SERVER_BUNDLE_CACHE_PATH || path.resolve(__dirname, '.node-renderer-bundles'), |
There was a problem hiding this comment.
New host config fallback (RENDERER_HOST → 'localhost') has no test coverage. Low risk given it's a one-line env passthrough, but since a wrong default here would silently break same-GVC reachability in production (renderer binding to localhost instead of 0.0.0.0 inside the container), a quick unit test asserting the fallback behavior would be cheap insurance.
| # Workloads, like Dyno types on Heroku | ||
| - daily-task | ||
| - node-renderer | ||
| - rails |
There was a problem hiding this comment.
Minor inconsistency worth a one-line clarifying comment: setup_app_templates orders workloads as daily-task, node-renderer, rails, while app_workloads/deploy_order order them node-renderer, rails, daily-task. The comment above only explains "node-renderer before rails" — it'd help future readers to note explicitly that daily-task's position relative to the other two doesn't matter (since it's a cron workload with no startup dependency on the renderer), so there's no drift between the three lists' intents.
| if ENV["ROLLING_DEPLOY_TOKEN"].present? | ||
| config.rolling_deploy_adapter = ReactOnRailsPro::RollingDeployAdapters::Http | ||
| config.rolling_deploy_token = ENV.fetch("ROLLING_DEPLOY_TOKEN") | ||
| config.rolling_deploy_previous_urls = ENV["ROLLING_DEPLOY_PREVIOUS_URLS"].presence | ||
| end |
There was a problem hiding this comment.
The Gemfile still pins react_on_rails_pro at 17.0.0.rc.9 — unchanged by this PR. ReactOnRailsPro::RollingDeployAdapters::Http, config.rolling_deploy_adapter=, config.rolling_deploy_token=, and config.rolling_deploy_previous_urls= are new API surface this PR depends on. Since this is a private gem I can't inspect its source from here to confirm the RC already ships this API.
If it doesn't, this raises NoMethodError at boot in every environment where ROLLING_DEPLOY_TOKEN is populated — staging, production, and every review app — since app.yml wires the secret unconditionally via cpln://secret/{{APP_SECRETS}}.ROLLING_DEPLOY_TOKEN. That's a hard boot crash, not a graceful degradation.
Worth double-checking that 17.0.0.rc.9 (or whatever SHA/tag is actually resolved) includes the rolling-deploy adapter before merging, and considering a version bump/comment pinning the minimum required release if not already covered by CI.
| ports: | ||
| - number: 3800 | ||
| # The React on Rails Pro Node Renderer speaks cleartext HTTP/2 (h2c). | ||
| # Rails/Thruster stays `http`; this renderer port intentionally differs. |
There was a problem hiding this comment.
With defaultOptions.autoscaling.maxScale: 1 and a single RENDERER_WORKERS_COUNT (set to 1 in app.yml), this workload has zero redundancy for SSR rendering — one replica, one worker. Combined with config.renderer_use_fallback_exec_js = false in the Rails initializer (fallback to ExecJS is intentionally disabled), any hiccup in this single instance (OOM, crash, slow restart) will make SSR fail hard for the whole app rather than degrade gracefully, and there's no second replica to absorb the outage while it recovers.
This is probably an acceptable tradeoff for a low-traffic tutorial/demo app, but since the same app.yml/node-renderer.yml templates apply to the production alias too (no override in controlplane.yml), it's worth confirming this single-worker/single-replica setup is intentional for the production example as well, not just cost-driven for review apps.
Review SummaryThis PR rolls the React on Rails Pro Node Renderer out as its own Control Plane workload ( What's solid
Points worth a second look (posted as inline comments)
SecurityNo significant concerns. Secrets ( Performance
Nothing here blocks merging; the flagged items are hardening/clarity suggestions, not correctness bugs. |
Review SummaryThis is a well-structured infra change that splits the React on Rails Pro Node Renderer into its own Control Plane workload, ordered ahead of Rails via cpflow's new What it does
Findings (posted inline)
Nice to see
|
✅ Review App DeletedReview app for PR #777 is deleted |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/initializers/react_on_rails_pro_spec.rb (1)
28-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a negative test case for absent
ROLLING_DEPLOY_TOKEN.The two tests cover token-present scenarios well. A third test verifying that when
ROLLING_DEPLOY_TOKENis absent (or blank), the rolling-deploy adapter remains unconfigured would guard against future regressions where the guard condition is accidentally removed or weakened.💡 Suggested additional test
it "does not configure rolling deploy when the token is absent" do ENV.delete("ROLLING_DEPLOY_TOKEN") ENV.delete("ROLLING_DEPLOY_PREVIOUS_URLS") load Rails.root.join("config/initializers/react_on_rails_pro.rb") config = ReactOnRailsPro.configuration expect(config.rolling_deploy_adapter).to be_nil expect(config.rolling_deploy_token).to be_nil end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/initializers/react_on_rails_pro_spec.rb` around lines 28 - 47, Extend the initializer specs with a negative case for missing or blank ROLLING_DEPLOY_TOKEN: clear the token and previous-URL environment variables, load the initializer, and assert ReactOnRailsPro.configuration leaves rolling_deploy_adapter and rolling_deploy_token unset. Keep the existing token-present and blank-URL scenarios unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/initializers/react_on_rails_pro_spec.rb`:
- Around line 28-47: Extend the initializer specs with a negative case for
missing or blank ROLLING_DEPLOY_TOKEN: clear the token and previous-URL
environment variables, load the initializer, and assert
ReactOnRailsPro.configuration leaves rolling_deploy_adapter and
rolling_deploy_token unset. Keep the existing token-present and blank-URL
scenarios unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef71d8da-4b4f-4df7-9876-766680a67622
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.controlplane/Dockerfile.controlplane/controlplane.yml.controlplane/docs/cpflow-vs-terraform.md.controlplane/docs/testing-cpflow-github-actions.md.controlplane/readme.md.controlplane/shakacode-team.md.controlplane/templates/app.yml.controlplane/templates/node-renderer.yml.controlplane/templates/org.yml.controlplane/templates/rails.yml.github/cpflow-help.md.github/workflows/cpflow-cleanup-stale-review-apps.yml.github/workflows/cpflow-delete-review-app.yml.github/workflows/cpflow-deploy-review-app.yml.github/workflows/cpflow-deploy-staging.yml.github/workflows/cpflow-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.ymlGemfileconfig/initializers/react_on_rails_pro.rbrenderer/node-renderer.jsspec/initializers/react_on_rails_pro_spec.rb
1 participant
Summary
Control Plane can now roll the React on Rails Pro renderer as a separate workload before Rails. The renderer uses the same app image, runs the rolling-deploy boot seed before
yarn node-renderer, and Rails reaches it through same-GVC service DNS.The bundled
cpflowgem and generated cpflow workflows are pinned tov5.2.0, the release that supports ordered deploy groups and per-workloaddeploy-image -w/--workloaddeploys.Upstream prerequisites
Review follow-ups addressed
set -eso unexpected boot-seed failures block renderer readiness instead of falling through toyarn node-renderer.RENDERER_SERVER_BUNDLE_CACHE_PATH=/app/renderer/.node-renderer-bundlesand maderenderer/node-renderer.jsconsume it, so the Ruby boot seed and Node renderer use the same cache directory in copy mode.node-rendereronprotocol: http2after checkingreact-on-rails-pro-node-renderer@17.0.0-rc.9; its worker starts Fastify with cleartext HTTP/2 and listens with{ port, host }.50m,512Mi, one worker) for tutorial cost control, documented that choice, and expanded the startup probe window to 120 seconds for Rails boot plus previous-bundle fetches.ROLLING_DEPLOY_PREVIOUS_URLSwith.presence.ROLLING_DEPLOY_TOKENis present.argsdistinction and Terraform comparison snippets.Validation
bin/conductor-exec bundle exec cpflow version→5.2.0bin/conductor-exec bin/test-cpflow-github-flow bundle exec cpflowbin/conductor-exec ruby -S cpflow doctorbin/conductor-exec bundle exec cpflow config -a react-webpack-rails-tutorial-stagingbin/conductor-exec bundle exec cpflow config -a react-webpack-rails-tutorial-productionbin/conductor-exec bundle exec cpflow config -a qa-react-webpack-rails-tutorial-1bin/conductor-exec ruby -c config/initializers/react_on_rails_pro.rbbin/conductor-exec node -c renderer/node-renderer.jsbin/conductor-exec bundle exec rspec spec/initializers/react_on_rails_pro_spec.rbbin/conductor-exec bundle exec rubocopsetup_app_templates,app_workloads,deploy_order, renderer boot-seed ordering, rendererhttp2, startup probe window, shared renderer cache path, same-GVCRENDERER_URL,RENDERER_HOST, and rolling-deploy env wiring.react-on-rails-pro-node-renderer@17.0.0-rc.9confirmingRENDERER_HOST/hostconfig and h2c listener behavior.Local note: a full
bin/conductor-exec bundle exec rspecrun was not green in this checkout because generated SSR assets were absent (ssr-generated/server-bundle.js). The targeted initializer spec above passed; hosted RSpec remains the authoritative full-suite gate for this PR head.Docs follow-up
No separate reactonrails.com PR appears necessary for the upstream prerequisite: https://reactonrails.com/docs/pro/rolling-deploy-adapters/#deploy-the-renderer-before-rails already documents boot seeding and renderer-before-Rails ordering.
Post-Deploy Monitoring & Validation
Validation window: first review-app deploy after merge, then the next staging deploy and production promotion. Owner: deployment operator running the workflow.
Healthy signals:
cpflow deploy-imageoutput showsnode-rendererdeploy/wait completing beforerails.cpflow ps -a <app>showsnode-rendererandrailsready/latest-ready.cpflow logs -a <app> -w node-renderershowsreact_on_rails_pro:pre_seed_renderer_cachecompleting before renderer startup.Failure signals:
node-rendererstartup/readiness probe fails or never reaches ready./react_on_rails_pro/rolling_deploy.node-renderer.<app>.cpln.local:3800.Rollback/mitigation:
node-rendererfromapp_workloads/deploy_orderand restoreRENDERER_URL=http://localhost:3800if separate-workload rollout fails.ROLLING_DEPLOY_TOKENin the app secret dictionary with the same value visible to Rails and renderer, then redeploy.Summary by CodeRabbit
New Features
Documentation
Maintenance